Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add assets source filenames #125

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Add assets source filenames #125

merged 2 commits into from
Apr 3, 2024

Conversation

batistadasilva
Copy link
Contributor

@batistadasilva batistadasilva commented Mar 29, 2024 •
edited
Loading

Add sourceFilename field

Asset resources in webpack-stats.json file will look like,

"assets": {
  "assets/test-bbf3c94e2a3948c98900.txt": {
    "name": "assets/test-bbf3c94e2a3948c98900.txt",
    "path": "/home/user/project-root/assets/test-bbf3c94e2a3948c98900.txt",
    "publicPath": "http://localhost:3000/assets/test-bbf3c94e2a3948c98900.txt",
    "sourceFilename": "src/test.txt"
  }
}

Related to django-webpack/django-webpack-loader#343

@batistadasilva batistadasilva force-pushed the assets-source-filenames branch from 65e6859 to 0223bca Compare March 29, 2024 08:50
batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Mar 29, 2024
@batistadasilva
Handle transformed assets URL
7d52a83 
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
This was referenced Mar 29, 2024
Merged
Closed
fjsj
fjsj reviewed Apr 1, 2024
View reviewed changes
lib/index.js Show resolved Hide resolved
fjsj
fjsj requested changes Apr 1, 2024
View reviewed changes
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but please check suggested improvement.

@batistadasilva batistadasilva force-pushed the assets-source-filenames branch from 0223bca to 4816591 Compare April 2, 2024 08:38
@batistadasilva batistadasilva requested a review from fjsj April 2, 2024 08:39
@batistadasilva
Copy link
Contributor Author

@fjsj Thanks for the review

batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Apr 2, 2024
@batistadasilva
Handle transformed assets URL
889c5b4 
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
batistadasilva added a commit to hostnfly/django-webpack-loader that referenced this pull request Apr 2, 2024
@batistadasilva
Handle transformed assets URL
65dddf2 
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes django-webpack#343

Depends on django-webpack/webpack-bundle-tracker#125
fjsj pushed a commit to django-webpack/django-webpack-loader that referenced this pull request Apr 2, 2024
@batistadasilva @fjsj
Handle transformed assets URL
bf59f53 
Adding the following template tag

```
{% webpack_asset 'path/to/original/file' %}
```

Fixes #343

Depends on django-webpack/webpack-bundle-tracker#125
fjsj
fjsj reviewed Apr 2, 2024
View reviewed changes
lib/index.js Outdated Show resolved Hide resolved
fjsj
fjsj requested changes Apr 2, 2024
View reviewed changes
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check comment.

@batistadasilva batistadasilva requested a review from fjsj April 2, 2024 13:09
@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

Some tests are breaking @batistadasilva
Could you please check?

@batistadasilva
Copy link
Contributor Author

@fjsj It seems like there is a broken test in master branch

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

The last build from 3 months ago did pass: https://github.com/django-webpack/webpack-bundle-tracker/actions/runs/7466611716
Are you seeing the same errors in localhost as well? Or in your fork?

@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 2, 2024 •
edited
Loading

I am seeing the same errors when running in localhost on the commit 97c7ec6

How can I help?

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

The error is this:

    -   "js/862.js",
    +   "js/75.js",

Please check if this is simply a hash change. If so, you can safely change the test assertion.

@batistadasilva
Copy link
Contributor Author

Please check if this is simply a hash change.

Yes it was

@fjsj
Copy link
Member

fjsj commented Apr 2, 2024

@batistadasilva I think now it breaks in Webpack 4? I think it's safe to ignore sourceFilename in Webpack 4. Whoever needs to use this should upgrade, as it's not a critical feature.

@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024 •
edited
Loading

@fjsj It seems like a typing error, but I don't know how to handle it

Could you help me wit this?

@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

If assetsInfo is undefined, you can ignore adding sourceFilename. Like:

if (stats.compilation.assetsInfo) {
  fileInfo.sourceFilename = stats.compilation.assetsInfo.get(assetName).sourceFilename;
}

@batistadasilva batistadasilva force-pushed the assets-source-filenames branch from 689c4da to 3b0306f Compare April 3, 2024 13:11
@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024 •
edited
Loading

@fjsj Done

However, the typing errors may still remain

@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

@batistadasilva I see that's a tsc error, but we can ignore it on Webpack 4.

Please enable me to edit this PR:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Or look for a way to ignore the error TS2339 in those two lines.

@batistadasilva
Add assets source filenames
fcdad69 
Asset resources in `webpack-stats.json` file will look like,

```json
"assets": {
  "assets/test-bbf3c94e2a3948c98900.txt": {
    "name": "assets/test-bbf3c94e2a3948c98900.txt",
    "path": "/home/user/project-root/assets/test-bbf3c94e2a3948c98900.txt",
    "publicPath": "http://localhost:3000/assets/test-bbf3c94e2a3948c98900.txt",
    "sourceFilename": "src/test.txt"
  }
}
```

Related to django-webpack/django-webpack-loader#343
@batistadasilva
fix chunk name
3ffd7ce
@batistadasilva batistadasilva force-pushed the assets-source-filenames branch from 3b0306f to 3ffd7ce Compare April 3, 2024 17:56
@batistadasilva
Copy link
Contributor Author

batistadasilva commented Apr 3, 2024 •
edited
Loading

Please enable me to edit this PR:

It would be with pleasure, but I couldn't find the checkbox the documentation is talking about

Or look for a way to ignore the error TS2339 in those two lines.

I don't think we can ignore a specific TS rule, so I ignored TS in both lines.
I hope it's fine.

@fjsj fjsj merged commit b0a7c27 into django-webpack:master Apr 3, 2024
3 checks passed
@fjsj
Copy link
Member

fjsj commented Apr 3, 2024

Thanks! We will release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjsj fjsj fjsj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@batistadasilva @fjsj